-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dense matching based line matcher #87
base: main
Are you sure you want to change the base?
Conversation
Merging the PR in roma main, just gotta check no regressions first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Able to run the test in a fresh install. Would be better to add dependency to RoMa, and maybe change the path in the testing script to command line args?
@@ -0,0 +1,30 @@ | |||
import os | |||
|
|||
import romatch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
romatch is not included in requirements.txt or dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we making a separate installation, as we now already make HAWP, LBD separately installed to reduce dependencies.
Added support for RoMa inspired by its keypoint matcher here: https://github.com/Parskatt/RoMa/blob/main/romatch/models/matcher.py#L576. Currently I rely on this PR: Parskatt/RoMa#73 so one needs to RoMa from that branch. @Parskatt
For now I just support a simple one-way nearest neighbor in the end. Ideally we could do one-to-many in the end of the matching. Attached are the results after warping the lines from DeepLSD:
Added a test script here: https://github.com/cvg/limap/blob/features/dense_matching/scripts/test_matching.py
More feedbacks and tests are very welcome here.